feat(mdns): emit ToSwarm::NewExternalAddrOfPeer
#5623
Conversation
protocols/mdns/src/behaviour.rs
Outdated
| @@ -290,6 +296,11 @@ where | |||
| &mut self, | |||
| cx: &mut Context<'_>, | |||
| ) -> Poll<ToSwarm<Self::ToSwarm, THandlerInEvent<Self>>> { | |||
| // If there are pending addresses to be emitted we emit them. | |||
| if let Some(event) = self.pending_events.pop_front() { | |||
There was a problem hiding this comment.
why do you prefer having this here instead of after returning discovered addresses?
I feel it's more correct to do this after returning the addresses and maintaining the previous flow of the pool loop.
There was a problem hiding this comment.
Thanks for your comments.
However, my idea here is that Event::Discovered and ToSwarm::NewExternalAddrOfPeer events are related.
After Event::Discovered is returned in
rust-libp2p/protocols/mdns/src/behaviour.rs
Line 354 in 9a45db3
poll will return.When
poll is entered next time, the NewExternalAddrOfPeer event will be returned because there is a NewExternalAddrOfPeer event in the queue.
The result is similar to this:
receive mdns::Event::Discovered Event
mDNS discovered a new peer: 12D3KooWGkhG2wR1mtadqShqiVEBjLHo8xk48ULrbkdftMsBbttR
NewExternalAddrOfPeer: /ip4/192.168.175.1/udp/63808/quic-v1/p2p/12D3KooWGkhG2wR1mtadqShqiVEBjLHo8xk48ULrbkdftMsBbttR, 12D3KooWGkhG2wR1mtadqShqiVEBjLHo8xk48ULrbkdftMsBbttR
There was a problem hiding this comment.
However, my idea here is that Event::Discovered and ToSwarm::NewExternalAddrOfPeer events are related.
yeah exactly, that's why if we discover multiple addresses we shouldn't return all the NewExternalAddrOfPeer events before the second and subsequent Event::Discovered(discovered) right?
There was a problem hiding this comment.
Thank you for your detailed explanation.
I have placed the return time of the NewExternalAddrOfPeer event after the Event::Discovered event.
Please check whether it meets your expectations.
There was a problem hiding this comment.
This is interesting, I just found what may be a bug, even if we discover multiple nodes and insert them in discovered we only return the first one added as discovered is a scope variable, right? So what I suggest is dropping discovered and just push everything to pending events, that way we'll have the ToSwarm::GenerateEvent(Event::Discovered(event)) ToSwarm::NewExternalAddrOfPeer events returned sequentially wdyt?
This is indeed a very good suggestion. |
Description
implement #5104
Notes & open questions
Change checklist